Skip to content

[libc++] Use __alloc_traits in <deque> whenever it is available for consistency #126595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Mar 19, 2025
Merged

[libc++] Use __alloc_traits in <deque> whenever it is available for consistency #126595

merged 3 commits into from
Mar 19, 2025

Conversation

@winner245
Copy link
Contributor

@winner245 winner245 commented Feb 10, 2025 •
edited
Loading

When an allocator-aware container already defines a member type alias __alloc_traits for std::allocator_traits<allocator_type>, we should consistently use __alloc_traits. Mixing the usage of both __alloc_traits and std::allocator_traits can lead to inconsistencies and confusion.

The following are some specific instances from deque with mixed usages of __alloc_traits and allocator_traits<allocator_type>:

llvm-project/libcxx/include/deque

Lines 62 to 64 in 3e62321

noexcept((__alloc_traits::propagate_on_container_move_assignment::value &&
is_nothrow_move_assignable<allocator_type>::value) ||
allocator_traits<allocator_type>::is_always_equal::value);

llvm-project/libcxx/include/deque

Lines 678 to 680 in 3e62321

(__alloc_traits::propagate_on_container_move_assignment::value &&
is_nothrow_move_assignable<allocator_type>::value) ||
allocator_traits<allocator_type>::is_always_equal::value);

llvm-project/libcxx/include/deque

Lines 1383 to 1385 in 3e62321

(__alloc_traits::propagate_on_container_move_assignment::value &&
is_nothrow_move_assignable<allocator_type>::value) ||
allocator_traits<allocator_type>::is_always_equal::value) {

This PR propose to consistently use __alloc_traits.

@github-actions
Copy link

github-actions bot commented Feb 10, 2025 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

frederick-vs-ja
frederick-vs-ja reviewed Feb 11, 2025
View reviewed changes
@winner245 winner245 marked this pull request as ready for review February 11, 2025 17:30
@winner245 winner245 requested a review from a team as a code owner February 11, 2025 17:30
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 11, 2025

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

When an allocator-aware container already defines a member type alias __alloc_traits for std::allocator_traits&lt;allocator_type&gt;, we should consistently use __alloc_traits. Mixing the usage of both __alloc_traits and std::allocator_traits can lead to inconsistencies and confusion.

Full diff: https://github.com/llvm/llvm-project/pull/126595.diff

3 Files Affected:

  • (modified) libcxx/include/deque (+4-4)
  • (modified) libcxx/include/forward_list (+2-2)
  • (modified) libcxx/include/list (+2-2)
diff --git a/libcxx/include/deque b/libcxx/include/deque
index 95200b4801d7ff3..ce29286445d4c45 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -61,7 +61,7 @@ public:
     deque& operator=(deque&& c)
         noexcept((__alloc_traits::propagate_on_container_move_assignment::value &&
                   is_nothrow_move_assignable<allocator_type>::value) ||
-                 allocator_traits<allocator_type>::is_always_equal::value);
+                 __alloc_traits::is_always_equal::value);
     deque& operator=(initializer_list<value_type> il);
 
     template <class InputIterator>
@@ -133,7 +133,7 @@ public:
     iterator erase(const_iterator p);
     iterator erase(const_iterator f, const_iterator l);
     void swap(deque& c)
-        noexcept(allocator_traits<allocator_type>::is_always_equal::value);  // C++17
+        noexcept(__alloc_traits::is_always_equal::value);  // C++17
     void clear() noexcept;
 };
 
@@ -677,7 +677,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI deque& operator=(deque&& __c) noexcept(
       (__alloc_traits::propagate_on_container_move_assignment::value &&
        is_nothrow_move_assignable<allocator_type>::value) ||
-      allocator_traits<allocator_type>::is_always_equal::value);
+      __alloc_traits::is_always_equal::value);
 
   _LIBCPP_HIDE_FROM_ABI void assign(initializer_list<value_type> __il) { assign(__il.begin(), __il.end()); }
 #  endif // _LIBCPP_CXX03_LANG
@@ -1382,7 +1382,7 @@ template <class _Tp, class _Allocator>
 inline deque<_Tp, _Allocator>& deque<_Tp, _Allocator>::operator=(deque&& __c) noexcept(
     (__alloc_traits::propagate_on_container_move_assignment::value &&
      is_nothrow_move_assignable<allocator_type>::value) ||
-    allocator_traits<allocator_type>::is_always_equal::value) {
+    __alloc_traits::is_always_equal::value) {
   __move_assign(__c, integral_constant<bool, __alloc_traits::propagate_on_container_move_assignment::value>());
   return *this;
 }
diff --git a/libcxx/include/forward_list b/libcxx/include/forward_list
index 4b6ca8ea8587c08..79d144e6421064d 100644
--- a/libcxx/include/forward_list
+++ b/libcxx/include/forward_list
@@ -719,7 +719,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI forward_list& operator=(forward_list&& __x) noexcept(
       (__node_traits::propagate_on_container_move_assignment::value &&
        is_nothrow_move_assignable<allocator_type>::value) ||
-      allocator_traits<allocator_type>::is_always_equal::value);
+      __node_traits::is_always_equal::value);
 
   _LIBCPP_HIDE_FROM_ABI forward_list& operator=(initializer_list<value_type> __il);
 
@@ -1013,7 +1013,7 @@ template <class _Tp, class _Alloc>
 inline forward_list<_Tp, _Alloc>& forward_list<_Tp, _Alloc>::operator=(forward_list&& __x) noexcept(
     (__node_traits::propagate_on_container_move_assignment::value &&
      is_nothrow_move_assignable<allocator_type>::value) ||
-    allocator_traits<allocator_type>::is_always_equal::value) {
+    __node_traits::is_always_equal::value) {
   __move_assign(__x, integral_constant<bool, __node_traits::propagate_on_container_move_assignment::value>());
   return *this;
 }
diff --git a/libcxx/include/list b/libcxx/include/list
index 3fcf796ebc03d04..9cdb7ffd78af02b 100644
--- a/libcxx/include/list
+++ b/libcxx/include/list
@@ -731,7 +731,7 @@ public:
   _LIBCPP_HIDE_FROM_ABI list& operator=(list&& __c) noexcept(
       (__node_alloc_traits::propagate_on_container_move_assignment::value &&
        is_nothrow_move_assignable<__node_allocator>::value) ||
-      allocator_traits<allocator_type>::is_always_equal::value);
+      __node_alloc_traits::is_always_equal::value);
 
   _LIBCPP_HIDE_FROM_ABI list& operator=(initializer_list<value_type> __il) {
     assign(__il.begin(), __il.end());
@@ -1070,7 +1070,7 @@ template <class _Tp, class _Alloc>
 inline list<_Tp, _Alloc>& list<_Tp, _Alloc>::operator=(list&& __c) noexcept(
     (__node_alloc_traits::propagate_on_container_move_assignment::value &&
      is_nothrow_move_assignable<__node_allocator>::value) ||
-    allocator_traits<allocator_type>::is_always_equal::value) {
+    __node_alloc_traits::is_always_equal::value) {
   __move_assign(__c, integral_constant<bool, __node_alloc_traits::propagate_on_container_move_assignment::value>());
   return *this;
 }

@philnik777
Copy link
Contributor

philnik777 commented Feb 12, 2025

These changes are modifying stuff that was 1:1 standardese I think, so I'm not sure this change leads to any less "confusion".

@winner245
Copy link
Contributor Author

winner245 commented Feb 12, 2025

These changes are modifying stuff that was 1:1 standardese I think, so I'm not sure this change leads to any less "confusion".

For deque, we were mixing __alloc_traits and allocator_traits<allocator_type> in the same code block in several cases, which is a clear inconsistency. Since we've already defined the member type __alloc_traits, I believe consistently using __alloc_traits would reduce confusion.

For list and forward_list, we were mixing __node_alloc_traits (list) or __node_traits (forward_list) with allocator_traits<allocator_type>, which implicitly assumes that rebound allocator traits and allocator_traits<allocator_type> have the same properties. Once LWG3267 is fixed, I believe fixing the inconsistency here would reduce confusion. However, for now I do not want to get into these subtilties due to the implicit assumption made in our implementation. Therefore, I've reverted the changes for list and forward_list.

@philnik777
Copy link
Contributor

philnik777 commented Feb 12, 2025

Just to be clear, I'm really ambivalent on whether we should make these changes.

@ldionne ldionne changed the title [libc++] Use __alloc_traits whenever it is available for consistency [libc++] Use __alloc_traits in <deque> whenever it is available for consistency Mar 19, 2025
ldionne
ldionne approved these changes Mar 19, 2025
View reviewed changes
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a suggestion.

@ldionne ldionne merged commit 617646a into llvm:main Mar 19, 2025
11 of 18 checks passed
@winner245 winner245 deleted the alloc_traits branch March 19, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@ldionne ldionne ldionne approved these changes

Assignees

No one assigned

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@winner245 @llvmbot @philnik777 @ldionne @frederick-vs-ja